[MINOR] Clean up of isDebugEnabled() usages#2435
Conversation
| if(LOG.isDebugEnabled()) | ||
| LOG.debug("Begin Function "+fkey); | ||
|
|
||
| LOG.debug("Begin Function " + fkey); |
There was a problem hiding this comment.
The main difference here, is that the LOG.isDebugEnabled() is very cheap, while the string concatenation can be very expensive. Unfortunately to call the LOG.debug java has to resolve the string variable input, and that is the main time used, especially if Logging is disabled.
I do not know if there is a way to avoid the string concatenation, or somehow getting it delayed to after the logging is analysed to be off.
There was a problem hiding this comment.
I see your point, but isnt it in practice in micro-level noise for these simple concats? i think we might over-engineer here
Why are we sticking to apache commons logging? with the newer SLF4J we would could do stuff like this:
LOG.debug("Begin Function {}", fkey);also SLF4J might already be used inside apachae commons logging, since it's already in the pom
There was a problem hiding this comment.
This would be cool, but we should not change the logging framework, so we just need to verify.
e-strauss
force-pushed
the
is-debug-clean-up
branch
from
940746b to
423df5c
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2435 +/- ##
============================================
- Coverage 71.55% 71.44% -0.11%
- Complexity 47461 48786 +1325
============================================
Files 1539 1572 +33
Lines 182631 188914 +6283
Branches 35919 37016 +1097
============================================
+ Hits 130677 134969 +4292
- Misses 41944 43529 +1585
- Partials 10010 10416 +406 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
e-strauss
force-pushed
the
is-debug-clean-up
branch
2 times, most recently
from
cd23f73 to
d61285d
Compare
Follow-up to apache#2432. Adds org.apache.sysds.utils.ParameterizedLogger, a commons-logging adapter with `{}` and `{%fmt}` placeholders that absorbs the isEnabled() guard, so `if(LOG.isDebugEnabled()) LOG.debug( String.format(...))` collapses to `LOG.debug("... {%fmt}", arg)`. Refactors call sites and adds a unit test covering format() branches and dispatch.
e-strauss
force-pushed
the
is-debug-clean-up
branch
from
506b17b to
eaf69bb
Compare
2 participants
This PR is a followup to #2432 .
We have 131 Usages of LOG.isDebugEnabled()
This PR adds a new utils for Parameterized Logging
This helps with readability and simplifies code coverage